-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change client_id_scheme
to a prefix
#263
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the cleanest approach that solves main security problem while not breaking other specifications like OpenID Federation and DIDs, while following OAuth 2.0 principle of client_id/aud uniquely identifying the client throughout the transaction
Co-authored-by: Joseph Heenan <[email protected]>
Thanks Daniel! One final thought: I am dubious about adding the prefix to pre-registered client ids. I don't think it actually really matters for VP, but I think it creates a clear gap as there's no way it could be accepted in normal OAuth, and if might be better if we chose something that could potentially eventually make it into an OAuth standard. Would removing the prefix for pre-registered clients and saying this work:
|
@@ -457,9 +471,9 @@ Location: https://client.example.org/universal-link? | |||
8%22%5D%7D%7D%7D | |||
``` | |||
|
|||
* `entity_id`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. | |||
* `https`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Since the Entity Identifier already is defined to start with `https:`, the `https` does not need to be prefixed additionally. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `https`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Since the Entity Identifier already is defined to start with `https:`, the `https` does not need to be prefixed additionally. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`. | |
* `https`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Since the Entity Identifier already is defined to start with `https:`, the `https` is not prefixed additionally. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to prescribe OpenID Federation as the only way metadata could be resolved from the client ID when it is an HTTPS url. There are multiple well-known metadata locations proposed for client metadata and a similar precedent exists within OAuth/OpenID for IDP / Authorisation Server metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is that federation is the only thing that uses the https client id scheme, and this doesn't stop a well-known based client id scheme being created, it would just use a client_id like wellknown:https://rp.example.com/
.
If you think the text isn't conveying this could you suggest alternative wording please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `https`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Since the Entity Identifier already is defined to start with `https:`, the `https` does not need to be prefixed additionally. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`. | |
* `https`: This value indicates that the Clients metadata is available at a well-known location such as defined in [@!OpenID.Federation]. Processing rules for retrieving and validating the metadata for the well-known location being used, such as that given in [@!OpenID.Federation] MUST be followed. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't reflect the intention though. If people want to use https urls with a different resolution method, they use a client id scheme other than https, e.g. wellknown:https://rp.example.com/.
The "https" client-id scheme is reserved for federation only.
Avoiding the vagueness of "I'm going to somehow authenticate with you using this url, you go figure out how you think I'm trying to authenticate with you" is very much the point. If you think of the browser API, using the https client id scheme to reflect multiple ways the wallet can authenticate, can result in lots of deadends as the sandboxed wallet matcher is now unable to know if it supports the method the wallet is using to authenticate, because it literally doesn't know how the wallet is trying to authenticate and has no way to find out because it can't access the network.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@selfissued I'll change this to a "MUST NOT be prefixed" to be very clear.
@tplooker @jogu I agree with Joseph here. This is an opportunity to have a clearly defined mechanism both from a security point of view and from an implementation perspective. Unless there is a good reason to not prefix other https-based client id schemes, we should avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Federation is an overly complicated mechanism for a client to simply convey its metadata at a well-known location, I don't believe we should be promoting it as the preferred mechanism for clients that are choosing to identify via an HTTPS url which is what this proposal does. It relegates other HTTPS based url based client metadata discovery mechanism to requiring a double prefix style client id which I don't think is a clean solution.
If we would prefer to have less ambiguity then the text I suggested, I propose we have a seperate parameter to convey the metadata location for the client such as client_metadata_location=federation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tplooker Is it going to be any different than client_id_scheme
for https-based urls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, we are not promoting federation as a preferred mechanism. We are merely acknowledging that federation is already the long established way of client's using self-allocated client_ids beginning with https: and that there's not really any option open to us of changing that position.
The alternative (following in the vein of this PR) is that federation is also prefixed, but that introduces interoperability issues / confusion and still doesn't solve the 'clean solution' problem.
I think if we do want to do something that is a generic scheme based on https urls (and I think that could be a good idea), we need to do that with a prefix, otherwise we're going to overlap and/or conflict with at least two other different specifications. It's simply not sustainable or sensible to have multiple specs trying to define self-assert client ids that are pure https urls. Even federation probably shouldn't be doing it, but that ship has mostly sailed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with requests for clarifying changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are quite a few files in the diagrams folder that use client_id_scheme & client_id - should those be changed as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do believe that this is the best/cleanest path forward to guarantee security / prevent downgrading attacks.
That being said, I do understand the concerns that have been brought up, but do believe we should be dealing with that and especially the optimization for re-using the same key for different trust mechanisms in a different issue/PR.
FYI, I restructured the section on client id schemes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am philosophically opposed to special treatment of DIDs and Federation.
If we're gonna prefix, prefix all the new stuff equally. And ":" needn't be the prefix indicator.
Also it causes breakage in the Appendix A. OpenID4VP profile for the W3C Digital Credentials API #263 (comment)
discussed on the WG call. need to clarify the behavior of client id in an unsigned request in the browser API since that is where client id is an https: just like in openid federation. please make suggestions. there were multiple opinions that DIDs are not a special treatment. |
I think this PR also resolves #29? |
I don't think so. I'll comment on the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still object to special treatment of OpenID Federation and W3C Decentralized identifiers but am approving in the interest of making progress.
I note that I relate to Brian's uncomfortableness to OpenID Federation's "special" treatment, while recognizing that changing openid federation document to enable |
This is quite a significant change, and it's taken well over 6 months to get to this point - but we now have 8 approvals on it and no request for changes. We've discussed this particular approach I believe on all the working group meetings for at least 2 weeks. Not everyone is happy but, other than/despite the above debate over whether federation should have the "https" prefix or not (and similar for 'did'), I believe we've reached a solid consensus that prefixing is a workable approach. We did ask Oliver for feedback too, but he's not added anything more, and I also sent a message on the mailing list on 23rd September asking for feedback by 1st Oct, and (other than comments on this PR) we've not received anything further - so as discussed on today's WG call - merging! A big thank you to everyone that contributed here - even if the WG didn't go with your suggestions I genuinely believe that all the different ideas we've had helped guide the working group towards what we ended up with! |
Is it though? That openid federation document has been under development for more than eight years now and is on its 39th draft. And apparently a federation wallet architecture specification is needed for it to federation to work with wallets anyway. Changes aren't possible there? While introducing a breaking change to everything else (except for DIDs of course)... I honestly don't see any justification for special treatment. But I've approved anyway #263 (review) |
Solves #124 by removing client_id_scheme as a separate parameter and making it a prefix instead.
entity_id
was renamed tohttps
in order to avoid breaking OpenID Federation.Fixes #124
Fixes #29